Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding webhook 'delete' command to remove non-namespaced objects #791

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Adding webhook 'delete' command to remove non-namespaced objects #791

merged 2 commits into from
Oct 30, 2024
+529 −9

Conversation

heyvister1
Copy link
Contributor

@heyvister1 heyvister1 commented Oct 14, 2024 •
edited
Loading

sriov-network-operator dynamically creates validating/mutating/clusterroles/clusterrolebinding cluster objects, for specified SriovOperatorConfig. These objects are not being cleaned once sriov operator is uninstalled (via Helm/Openshift).

The proposed PR is to use a small go binary within Helm pre-delete hook. This binary will delete default SriovOperatorConfig, assuming added SriovOperatorConfig finalizer will make sure sriov controller is deleting all generated cluster level objects (e.g. webhooks), prior to operator deployment object is removed by Helm

@github-actions GitHub Actions
Copy link

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@coveralls
Copy link

coveralls commented Oct 15, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 11577275447

Details

  • 55 of 83 (66.27%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.6%) to 45.531%
Changes Missing Coverage Covered Lines Changed/Added Lines %
controllers/sriovoperatorconfig_controller.go 19 25 76.0%
cmd/sriov-network-operator-config-cleanup/main.go 0 9 0.0%
cmd/sriov-network-operator-config-cleanup/cleanup.go 36 49 73.47%
Files with Coverage Reduction New Missed Lines %
controllers/drain_controller.go 1 68.06%
Totals Coverage Status
Change from base Build 11576125023: 0.6%
Covered Lines: 6775
Relevant Lines: 14880
💛 - Coveralls

@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch 3 times, most recently from 52d9671 to 693e48d Compare October 20, 2024 15:21
@heyvister1
Copy link
Contributor Author

/test-all

@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch 2 times, most recently from 13b3cda to e2440c4 Compare October 22, 2024 07:40
@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch 2 times, most recently from 2ee6dad to 56a8346 Compare October 22, 2024 12:06
@heyvister1 heyvister1 requested a review from zeeke October 22, 2024 12:06
@adrianchiris
Copy link
Collaborator

/test-all

@heyvister1 heyvister1 force-pushed the webhooks-k8s-objects-deletion branch 2 times, most recently from ec25c78 to 798f5e7 Compare October 22, 2024 14:14
@heyvister1 heyvister1 requested a review from zeeke October 23, 2024 08:38
adrianchiris
adrianchiris reviewed Oct 28, 2024
View reviewed changes
cmd/sriov-network-operator-config-cleanup/cleanup.go Outdated

// watching 'default' config deletion with context timeout, in case sriov-operator fails to delete 'default' config
watcher, err := sriovcs.SriovOperatorConfigs(namespace).Watch(ctx, metav1.ListOptions{Watch: true})
defer watcher.Stop()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if err is not nil, is it safe to stop the watcher ? or should the defer be moved after the error check ?

adrianchiris
adrianchiris reviewed Oct 28, 2024
View reviewed changes
Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM, need CI to be green.

added one last comment from my side , thx for the patience ! :)
before we merge please re-work the commits so they do not reflect the review interations but rather the changes done.

e.g

commit 1 : add finalizer to sriovoperatorconfig
commit 2: cleanup binary related changes

@adrianchiris adrianchiris mentioned this pull request Oct 29, 2024
Closed
@heyvister1
Copy link
Contributor Author

@zeeke could you please add your approval for this PR?

@zeeke zeeke merged commit 5009e99 into k8snetworkplumbingwg:master Oct 30, 2024
14 checks passed
@rollandf rollandf mentioned this pull request Oct 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeeke zeeke zeeke approved these changes

@rollandf rollandf Awaiting requested review from rollandf

@adrianchiris adrianchiris Awaiting requested review from adrianchiris

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@heyvister1 @coveralls @adrianchiris @zeeke @rollandf